test: mutation testing for Nostr event handling - #849

No actionable comments were generated in the recent review. 🎉

Walkthrough

Mutation testing now runs through a shared Makefile target with configurable test ports. Event acceptance checks use private helpers with expanded test coverage. Expiration-tag and global spam-gate behavior receive regression tests.

Changes

Nostr testing improvements

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 54112

This change adds mutation-testing coverage and test-environment controls without any identified merge-blocking correctness or production risk; it is merge-ready after normal checks and review.

Suggested reviewers: arkanoider, grunch

Poem

A rabbit runs mutants through Make,
While test ports change for each test’s sake.
Events gain checks, tags stay true,
Spam gates guard their state anew.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Both review comments addressed — one of them turned out to be the opposite of what it looked like

Pushed 3e05ed2 and e885508, kept separate since they are unrelated findings.

src/nip33.rs — the Custom("expiration") arm can't be tested, because it can't be reached

The suggestion was to add a companion test for the TagKind::Custom("expiration") arm. I wrote that test first and it failed on its own first assertion, which sent me to check the premise:

let t = Tag::custom(TagKind::Custom(Cow::Borrowed("expiration")), vec!["123456".to_string()]);
// kind = Expiration | is_expiration = true

nostr normalises the tag name at construction, so t.kind() can never return Custom("expiration"). The second arm of has_expiration_tag was unreachable — and that also makes it an equivalent mutant: deleting it changes nothing observable, so no test could ever have killed it. Adding a test there would have been writing an assertion that passes for the wrong reason.

Worth correcting one thing I had assumed too: this arm is not the production-load-bearing one. order_to_tags does build the tag as Tag::custom(TagKind::Custom("expiration"), ..), but by the time it reaches create_event it has already normalised to TagKind::Expiration — so production has always taken the first arm.

So I deleted the dead arm instead, and added a test that pins the real path end to end: the exact Tag::custom shape order_to_tags emits must normalise and suppress the auto-add, asserting the caller's 123456 is the only expiration tag on the event. Verified it actually bites by forcing has_expiration_tag = false — it goes red with left: ["1788381606", "123456"], the config auto-add stacked on top of the caller's tag.

Flagging this one as your call, since it is a production change inside a test: PR. I think it belongs here — an equivalent mutant is precisely the kind of thing a mutation-testing pass should surface, and leaving it in means leaving a mutant nobody can ever kill. But the smaller option is equally defensible: restore the arm with a comment saying it is unreachable, and keep this PR's diff purely additive. Say the word and I'll do that instead.

Either way the deletion is safe across every caller: create_event has five entry points, all passing tags built internally, and the only two that carry an expiration are order_to_tags (Tag::custom, normalises) and price/manager.rs:528 (Tag::expiration, already the first arm).

Makefile — two of the three points hold

set -o pipefail: no change. Makefile:1 is SHELL := $(shell which bash), which applies to every recipe, and six existing targets already rely on it. Verified the recipe runs under bash.

Caller's port was being overwritten: fixed. Now MOSTRO_TEST_LN_PORT=$${MOSTRO_TEST_LN_PORT:-18080}, so a busy 18080 can be worked around without editing the Makefile.

Parallel workers sharing the port: fixed, and this was the one that mattered. Each worker runs the full suite in its own temp dir but shares the host's TCP ports, so two workers collide on the LNURL listener. Under mutation testing that collision is not just noise — a test failing for its own reasons counts as a killed mutant, so it silently inflates the very score this target exists to measure. Dropped to CARGO_MUTANTS_JOBS=1. Slower, but the number means something.

That failure mode is easy to see locally: a plain cargo test on this machine gives 1058 passed / 1 failed with AddrInUse on 8080, and MOSTRO_TEST_LN_PORT=18080 cargo test gives 1059 passed / 0 failed.

On the mutation score

Since this touches create_event, I re-ran the measurement on the new head rather than carry the old number over:

$ make mutation-test ARGS="--file src/app.rs --file src/nip33.rs -F 'in accept_event|in create_event'"
Found 9 mutants to test
ok       Unmutated baseline in 140s build + 41s test
9 mutants tested in 12m: 9 caught

0 missed, 0 timeout, 0 unviable.

It reads 9/9 now, not 10/10, and the PR body should say so. Removing the unreachable arm removes its || — and with it the replace || with && mutant — from the set. Nothing became uncovered.

While tracing that: 2 of the original 10 were delete field .. in check_trade_index, which land in the run because cargo-mutants 27.1.0's -F doesn't filter "delete field from struct expression" mutants. They are caught, so they never distorted the pass/fail, but the headline was really 8 targeted + 2 incidental. It is 7 + 2 now.

PR body updated to match — score line, the mutation-test invocation it quoted, and the cargo test count.

Verification

• cargo test: 1059 passed, 2 ignored (with MOSTRO_TEST_LN_PORT set past the host's busy 8080)
• cargo fmt --check: clean
• cargo clippy --all-targets --all-features -- -D warnings: clean

Rebased onto main — and the nostr 0.45 bump turned this PR's own guard test into the thing that verified it

This had gone CONFLICTING. It's now rebased onto current main (541129f), no merge commit, same six commits.

The conflict itself was three small hunks, but resolving them surfaced the real issue: this branch was on nostr-sdk 0.44.1 and main is now on nostr + nostr-sdk 0.45.1. That's a breaking change, so the rebase is also a small 0.44 → 0.45 migration.

What the migration touched

TagKind is gone from the 0.45 API. The branch had matches!(t.kind(), TagKind::Expiration); main has t.kind() == "expiration". I kept main's form — not as a tiebreak, but because TagKind genuinely doesn't exist in 0.45 (it's only in 0.44.x). Same for the test helpers: Tag::custom(TagKind::Custom(Cow::Borrowed("expiration")), ..) is now just Tag::custom("expiration", ..), matching how admin_cancel.rs and admin_settle.rs already write custom tags on main.

nostr_sdk::secp256k1 no longer resolves, and sign_schnorr now takes AsRef<[u8]> rather than a secp256k1::Message. So app.rs goes from sign_schnorr(&nostr_sdk::secp256k1::Message::from_digest([7u8; 32])) to sign_schnorr([7u8; 32]).

extract_lnurl returns Url, not String (from the LNURL scheme validation on main). The port-override test now asserts extracted.to_string() against the formatted URL, keeping both main's type and this branch's MOSTRO_TEST_LN_PORT override.

The part worth flagging

Commit 45ca902 adds a_custom_named_expiration_tag_normalises_and_suppresses_the_auto_add, and the comment I wrote on it 11 days ago says:

If an sdk upgrade ever stopped normalising, the auto-add would start firing on top of the caller's tag and this test goes red.

The sdk upgrade arrived. The test is green against 0.45 — nostr still normalises a custom-named expiration tag to the canonical NIP-40 kind, so has_expiration_tag's single check is still correct and order events are not double-stamping expirations. That's the one behavioural question the 0.45 bump raised in this file, and it's now answered rather than assumed.

I rewrote that comment to describe the behaviour instead of the (now non-existent) TagKind variants, so it doesn't rot again.

Verification on the rebased tree

• cargo fmt --check — clean
• cargo clippy --all-targets -- -D warnings — clean
• cargo test --bin mostrod — 1189 passed, 0 failed, 2 ignored

One note on that test run: on a first pass, lightning::invoice::tests::test_lnurl_validation_with_test_server failed with AddrInUse on port 8080 — which is precisely what commit 88679a0 in this PR exists to fix. Re-running with MOSTRO_TEST_LN_PORT=18080 gives a clean 1189/0. The failure was a live demonstration of the problem the commit addresses, not a regression.

Diff against main is unchanged in shape: 339 insertions, 13 deletions, same seven files.